Make resample kernel generation faster #2415
Conversation
1. loops are replaced with broadcasting 2. device is cast at end of kernel creation 3. dtype is defaulted to float32 ( float64) does nothing valuable 4. defaults for beta and dtype are moved to function declaration
|
Im not sure that the failures are in my code... i didnt change the style. |
|
Hi @xvdp! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
|
Thanks for the PR! Can you rebase the PR and also separate out the changes into separate PRs? The unit test failures were addressed in #2422 (lint test is known to be failing so can ignore that for now). And the reasoning for separating the PR is so each change is contained and easier to review, and since we likely won't be ready to merge all changes at the same time (as mentioned on #2414, we would need more data to back up the device casting). |
| @@ -5,6 +5,7 @@ | |||
| import warnings | |||
| from collections.abc import Sequence | |||
| from typing import Optional, Tuple, Union | |||
| from numpy import roll | |||
There was a problem hiding this comment.
Is this used? NumPy is not mandatory requirement of torchaudio, so we would like to avoid the use of NumPy.
| device: torch.device = torch.device("cpu"), | ||
| dtype: Optional[torch.dtype] = None, | ||
| dtype: Optional[torch.dtype] = torch.float32, |
There was a problem hiding this comment.
Can we keep the dtype effectiveness? The reason resample supports torch.float64 for the sake of use cases other than DL, which requires higher precision for quality.
|
re: +from numpy import roll Is this used?
…-- thats a booboo = sorry, something in my vscode environment that added
this. I will correct and resubmit.
re : Can we keep the dtype effectiveness?:
-- then you should just pass the dtype no? - I tried passing wavs as float
64 and saw no difference in the result, but did not do a full
through check.
Ill look at this again and see how to keep it.
On Wed, Jun 1, 2022 at 9:12 AM moto ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In torchaudio/functional/functional.py
<#2415 (comment)>:
> device: torch.device = torch.device("cpu"),
- dtype: Optional[torch.dtype] = None,
+ dtype: Optional[torch.dtype] = torch.float32,
Can we keep the dtype effectiveness? The reason resample supports
torch.float64 for the sake of use cases other than DL, which requires
higher precision for quality.
—
Reply to this email directly, view it on GitHub
<#2415 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AJYHPOVHPFVUREE4EVV7LUTVM6DVDANCNFSM5XC3AHDA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
mthrok
changed the title
|
@xvdp Do you have any update? Let me know if you are busy, we will look into it on our side. |
Summary: Modification from pull request #2415 to improve resample. Benchmarked for a 89% time reduction, tested in comparison to original resample method. Pull Request resolved: #2553 Reviewed By: carolineechen Differential Revision: D37997533 Pulled By: skim0514 fbshipit-source-id: ef4b719450ac26794db6ea01f9882509f4fda5cf
|
Addressed in #2553 |
|
Caroline, I will look at this, thanks.
…On Wed, Jun 1, 2022 at 8:48 AM Caroline Chen ***@***.***> wrote:
Thanks for the PR! Can you rebase the PR and also separate out the changes
into separate PRs? The unit test failures were addressed in #2422
<#2422> (lint test is known to be
failing so can ignore that for now). And the reasoning for separating the
PR is so each change is contained and easier to review, and since we likely
won't be ready to merge all changes at the same time (as mentioned on
#2414 <#2414>, we would need more
data to back up the device casting).
—
Reply to this email directly, view it on GitHub
<#2415 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AJYHPOWBYTAIPXT46LDGLI3VM6A47ANCNFSM5XC3AHDA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
Hi @xvdp, thanks again for the suggestions. Since we did not get a response in July, we went ahead and pulled out broadcasting component from your PR in this one (#2553) and merged the change. We got an 89% speedup as mentioned from this, thanks! For change 2 (device is cast at the end), we did not have the bandwidth to look into that change and potential improvements from it yet, so feel free to look into that. For change 3 (using float32 instead of 64) and 4 (setting defaults in func declaration), we would like to stick to using the current approach. For float64 default, there may be dsp use cases where more specific dtype is necessary, and to avoid breaking backwards compatibility. And for the beta default, since not all resampling methods require a beta, we would like to leave it as |
Changes:
Fix #2414